Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cli,Desktop,Mobile: Fix sync reported as in progress if an error occurs while removing sync locks #11188

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Oct 9, 2024

Summary

This pull request refactors Synchronizer.start to ensure that its cleanup logic runs, even if there is an uncaught Error. This fixes an issue originally reported on the forum — an error while removing sync locks could prevent sync cleanup logic from running.

Testing plan

Locally, I have verified that the following automated test passes:

Show test
diff --git a/packages/lib/services/synchronizer/Synchronizer.basics.test.ts b/packages/lib/services/synchronizer/Synchronizer.basics.test.ts
index 044649dfb..33d3544c3 100644
--- a/packages/lib/services/synchronizer/Synchronizer.basics.test.ts
+++ b/packages/lib/services/synchronizer/Synchronizer.basics.test.ts
@@ -8,6 +8,7 @@ import WelcomeUtils from '../../WelcomeUtils';
 import { NoteEntity } from '../database/types';
 import { fetchSyncInfo, setAppMinVersion, uploadSyncInfo } from './syncInfoUtils';
 import { ErrorCode } from '../../errors';
+import { LockType } from './LockHandler';
 
 describe('Synchronizer.basics', () => {
 
@@ -461,6 +462,35 @@ describe('Synchronizer.basics', () => {
 		expect(remotes.find(r => r.path === `${note.id}.md`)).toBeTruthy();
 	}));
 
+	it('should recover from failures to release sync locks', async () => {
+		await synchronizer().start();
+
+		const lockHandler = synchronizer().lockHandler();
+
+		let allowLockRelease = false;
+
+		// Use .spyOn to simulate an error that might, for example, be caused by network
+		// failure.
+		const spy = jest.spyOn(lockHandler, 'releaseLock').mockImplementation(async (type) => {
+			if (type === LockType.Sync && !allowLockRelease) {
+				throw new Error('Testing lock release failure');
+			}
+		});
+
+		let syncError: Error|null = null;
+		try {
+			await synchronizer().start();
+		} catch (error) {
+			syncError = error;
+		}
+		expect(syncError).not.toBeNull();
+		spy.mockClear();
+
+		// Should sync successfully, after failure
+		allowLockRelease = true;
+		await synchronizerStart();
+	});
+
 	it('should throw an error if the app version is not compatible with the sync target info', (async () => {
 		await synchronizerStart();

This test is not included in this pull request because it relies on .spyOn to mock a method of LockHandler. This seems fragile and likely to break in the future, particularly if sync locks are changed significantly or removed.

Additionally, I have verified that a Dropbox sync completes successfully and reports the number of fetched and created items.

@personalizedrefrigerator personalizedrefrigerator changed the title Cli,Desktop,Mobile: Fix errors removing sync locks break future syncs Cli,Desktop,Mobile: Fix errors removing sync locks breaks sync until app restarts Oct 9, 2024
@personalizedrefrigerator personalizedrefrigerator changed the title Cli,Desktop,Mobile: Fix errors removing sync locks breaks sync until app restarts Cli,Desktop,Mobile: Fix sync reported as in progress if an error occurs while removing sync locks Oct 9, 2024
@personalizedrefrigerator personalizedrefrigerator added the sync sync related issue label Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sync sync related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant